db/migrate/20241017143229_set_default_judge_for_repositories.rb (1)

3-3: Consider adding a comment to explain the significance of the default value.

The hardcoded value 17 might not be immediately clear to other developers. Consider adding a comment to explain what this value represents (e.g., the ID of the TESTed judge).

Here's a suggested change:

-    change_column_default :repositories, :judge_id, from: nil, to: 17
+    # Set the default judge_id to 17 (TESTed judge)
+    change_column_default :repositories, :judge_id, from: nil, to: 17

test/factories/repositories.rb (3)

9-9: LGTM! Consider adding a comment for clarity.

The schema information update correctly reflects the change in the database schema, setting the default judge_id to 17. This aligns with the PR objective of making TESTed the default judge for repositories.

Consider adding a comment to explain what judge_id 17 represents (e.g., TESTed) for better clarity:

-#  judge_id     :integer          default(17)
+#  judge_id     :integer          default(17) # Default to TESTed judge

---

Line range hint 21-25: Update factory definition to reflect new default behavior

The current factory definition explicitly sets judge to Judge.find(1), which overrides the new default value of 17 set in the schema. To better align with the schema change and accurately represent the new default behavior in tests, consider updating the factory definition.

Here's a suggested change:

 factory :repository do
   name { Faker::Lorem.word + Faker::Number.unique.number(digits: 8).to_s }
   remote { "[email protected]:dodona-edu/#{name}.git" }
-  judge { Judge.find(1) } # load python judge fixture
+  judge { Judge.find(17) } # load TESTed judge fixture (new default)
 end

Alternatively, if you want to keep the ability to easily switch between judges in tests, you could add a trait for the new default:

trait :default_judge do
  judge { Judge.find(17) } # TESTed judge
end

This way, you can use create(:repository) for the old default and create(:repository, :default_judge) for the new default in your tests.

---

Line range hint 27-37: Consider adding a trait for the new default judge

While the existing traits don't need to be updated, it might be beneficial to add a new trait for testing with the new default judge. This would provide more flexibility in tests and make it easier to create repositories with the new default judge.

Consider adding the following trait:

trait :default_judge do
  judge { Judge.find(17) } # TESTed judge (new default)
end

This would allow you to easily create repositories with the new default judge in your tests:

create(:repository, :default_judge)

It also maintains backwards compatibility, allowing you to still create repositories with the old default judge when needed:

create(:repository) # Uses Judge.find(1) as before

app/models/repository.rb (1)

Line range hint 227-227: Clarify judge fallback behavior in process_activities method

In the process_activities method, when updating an exercise, there's a fallback to judge if no judge is specified in the config:

act.judge = j || judge

With the new default judge_id, it's worth clarifying if this fallback should:

1. Remain as is (falling back to the repository's judge)
2. Use the new default judge (id: 17)
3. Be removed, allowing the default to be set at the database level

Consider updating this line to explicitly use the default judge if that's the intended behavior:

act.judge = j || judge || Judge.find_by(id: 17)

Alternatively, if the current behavior is intentional, consider adding a comment explaining why it doesn't use the default judge.

test/models/repository_test.rb (2)

9-9: LGTM. Consider adding a test for the default judge.

The change to set a default value of 17 for judge_id aligns with the PR objective to make TESTed the default judge for repositories. This modification ensures that new Repository instances will have TESTed as their default judge unless specified otherwise.

Consider adding a test case to verify that new Repository instances are created with the default judge (TESTed) when no judge is explicitly specified. This will help ensure the new default behavior is working as expected. Here's a suggested test case:

test 'should use TESTed as default judge when creating a new repository' do
  repository = create :repository, :git_stubbed
  assert_equal 17, repository.judge_id, "Expected new repository to have TESTed (judge_id: 17) as the default judge"
end

---

Line range hint 1-531: Consider updating existing tests to account for the new default judge.

While the schema change doesn't break any existing tests, it introduces a new default behavior that isn't explicitly tested. To ensure robustness and prevent potential issues in the future, consider the following suggestions:

1. Update tests that rely on a specific judge to explicitly set the judge, ensuring they're not accidentally using the new default.
2. Add assertions in relevant tests to verify that the correct judge is being used, especially in scenarios where the judge isn't explicitly set.

Here's an example of how you might update an existing test:

test 'should process correctly' do
  # Existing setup code...
  
  # Explicitly set the judge to ensure the test isn't relying on the default
  @repository.update(judge: @python)
  
  # Existing assertions...
  
  # Add an assertion to verify the correct judge is being used
  assert_equal @python, @repository.judge, "Expected repository to use the explicitly set judge"
end

These updates will help ensure that the tests remain accurate and effective with the new default judge behavior.

db/schema.rb (1)

13-13: Minor style improvements suggested by static analysis.

Consider the following style improvements:

1. Use underscores as thousands separators in the schema version number for better readability.
2. Use single quotes for the string literal in the judge_id default value assignment.

These are minor style suggestions and don't affect functionality.

Here's how you could apply these changes:

-ActiveRecord::Schema[7.2].define(version: 2024_10_17_143229) do
+ActiveRecord::Schema[7.2].define(version: 2024_10_17_143_229) do
   # ...
-    t.integer "judge_id", default: 17
+    t.integer 'judge_id', default: 17
   # ...

Also applies to: 393-393

🧰 Tools

🪛 rubocop

[convention] 13-13: Use underscores(_) as thousands separator and separate every 3 digits with them.

(Style/NumericLiterals)

Configuration used: CodeRabbit UI
Review profile: CHILL